feat: Porting PadLeft and PadRight from C##46
feat: Porting PadLeft and PadRight from C##46gavinbm wants to merge 9 commits intomicrosoft:mainfrom
Conversation
@microsoft-github-policy-service agree |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
shonk-msft
left a comment
shonk-msft
left a comment
There was a problem hiding this comment.
The description is a bit misleading. The number of characters added is just enough to get to the specified length, not the number specified in the second argument.
| @@ -238,6 +238,14 @@ Text.Replace("ABC", "X", s) | |||
| Text.Replace("ABC", "B", "X") | |||
| Text.Replace("ABACDAE", "A", "!!") | |||
|
|
|||
There was a problem hiding this comment.
There should be multiple kinds of test cases:
- Test with the named globals to ensure that errors are produced on bad input types, conversions are inserted where needed (eg, using
u2for the length), arity is handled correctly. - Test with special values to ensure that reduction happens as expected (once
ReduceCoreis implemented). Use enough cases that all the code inReduceCoreand in theExecXxxmethods is covered. - Tests that use "opt" types in slots that lift over opt. Eg, tests that use
qi8and something likequ2for the length.
| Text.Replace(s, s, s) | ||
|
|
||
| Text.Replace(s, "", s) | ||
|
|
There was a problem hiding this comment.
These tests aren't needed since they just duplicate what is above.
Tests here should be for lifting over sequence.
Also, we should add tensor based lifting tests, not just sequence.
| Text.Replace(N, "A", "B") | ||
| Text.Replace("A", N, "B") | ||
| Text.Replace("ABC", Wrap("B"), N) | ||
|
|
There was a problem hiding this comment.
Should also have:
- Tests for lifting.
- Tests of cases that aren't reduced during binding (once
ReduceCoreis implemented). That's the purpose of usingWrap.
shonk-msft
left a comment
shonk-msft
left a comment
There was a problem hiding this comment.
You'll need to run the tests and update the test baselines appropriately. Also see the comments about the types of tests needed.
2 participants
This PR adds support for calling the single argument version of PadLeft and PadRight to the Text functions. These functions take as arguments the strings they're operating on and the number of spaces to add as padding. Also have some basic tests, feedback on those is very much appreciated as I may be missing some things there.
C# docs for functions this PR includes:
https://learn.microsoft.com/en-us/dotnet/api/system.string.padleft?view=net-8.0#system-string-padleft(system-int32)
https://learn.microsoft.com/en-us/dotnet/api/system.string.padright?view=net-8.0#system-string-padright(system-int32)